[systeminfo] dynamic channels #13562

mherwege · 2022-10-17T20:11:59Z

This PR enhances the systeminfo binding to expose all devices on the system when multiple devices of the same type exist. So far, the binding only exposed the first of such devices.

These extra devices are exposed through dynamically created channels in dynamically created device groups. This will happen - for storage, drive, display, battery and network device groups. The new device groups will have an index appended (starting from 0). If no extra devices are detected, only the channel group without index will exist.
Extra fanspeed channels in the sensors group may also get created if the system has multiple fans.
Addition of a process channel group for the current process that is running OH. This makes monitoring the OH process easier, as the PID would change with a restart.

Signed-off-by: Mark Herwege <mark.herwege@telenet.be>

lolodomo · 2022-10-19T04:54:32Z

Looks like the integration tests are not working.

Signed-off-by: Mark Herwege <mark.herwege@telenet.be>

mherwege · 2022-10-19T07:29:45Z

Looks like the integration tests are not working.

@lolodomo I noticed and tried understanding why. I have made some adjustments, which solved part of the issues. But I am left with 3 tests that consistently fail with no changes to the code for the functionality in these tests. The tests are very similar to many other tests that work without problem. I can't figure it out.
I have disabled them for now. The test set runs successfully on my local machine with these 3 disabled. But I would like to get some help to understand what's going on here.

Update: I now see that the Java 17 build ran through, but not the Java 11 build, failing on one test. It looks like the mocked object doesn't return the mock value consistently.

Signed-off-by: Mark Herwege <mark.herwege@telenet.be>

mherwege · 2022-10-20T08:36:55Z

@lolodomo I found the problem with the CpuLoad1-5-15 update. It was introduced in the previous PR. I didn't notice as these channels are not available on Windows and I develop on Windows. Surprisingly, the integration tests did not fail with the previous PR, which is beyond my understanding.
I spend quit a bit of time trying to improve the integration test setup, and make that more resilient. I believe there were timing issues which sometimes made random tests fail, as evidenced by the failure for the Java 11 build in my previous commit. I hope the current version also resolves that.

Signed-off-by: Mark Herwege <mark.herwege@telenet.be>

lolodomo · 2022-10-20T11:37:52Z

Do you think you could have fixed #13524 ?

mherwege · 2022-10-20T14:16:45Z

Do you think you could have fixed #13524 ?

It sure looks similar to what I saw. Let's hope it has that effect.

I noticed the following line in the test setup code sometimes returned null, probably because the service was not registered in time. And this was not tested (there were no Null annotations and no assert).

systeminfoHandlerFactory = getService(ThingHandlerFactory.class, SysteminfoHandlerFactory.class);

I put an assert and waitForAssert around it, and I have not seen the problem since. I did similar things in other parts of the code as well. At least it should show a more relevant assert failure when it fails again.

lolodomo

Partial review (remains to review the thing handler and the unit tests).

You renamed a channel type which breaks the compatibility with already created things in UI.
I have the feeling that this renaming was not mandatory ?

bundles/org.openhab.binding.systeminfo/src/main/resources/OH-INF/thing/channels.xml

lolodomo · 2022-10-30T13:34:46Z

At my current review step, I am still not sure to understand why you need to create dynamically different thing types.
My feeling is that you can have still one unique thing type and when you create a thing from this type, you add dynamically new groups.
So it looks like you are creating a thing type for each thing and I do not really understand why.

jaywiseman1971 · 2022-10-30T13:52:07Z

"Addition of a process channel group for the current process that is running OH. This makes monitoring the OH process easier, as the PID would change with a restart."

I'm interested in this piece above. Have a recently posted to understand how to get the current binding to work as such w/o anybody replying.

https://community.openhab.org/t/oh-3-2-systeminfo-binding-process-pi4/140406

Best, Jay

mherwege · 2022-10-30T17:49:03Z

At my current review step, I am still not sure to understand why you need to create dynamically different thing types.
My feeling is that you can have still one unique thing type and when you create a thing from this type, you add dynamically new groups.
So it looks like you are creating a thing type for each thingroupsg and I do not really understand why.

It was my understanding you can add channels dynamically, but not channel groups. There are no methods for that. One needs to create a new thing type. If I would create the same thing type, I would run into a conflict, or cannot use the static definition at all anymore. I would be very happy to be proven wrong about this. I believe there are very few bindings creating channel groups (from the top of my head: mqtt).
I don’t have access to my PC at the moment, and won’t have time to look much onto this until next week.

lolodomo · 2022-10-31T08:27:33Z

It was my understanding you can add channels dynamically, but not channel groups. There are no methods for that. One needs to create a new thing type.

It looks like you are right. I don't know why we have this restriction in the core framework.

lolodomo

Review of changes in the thing handler

...teminfo/src/main/java/org/openhab/binding/systeminfo/internal/handler/SysteminfoHandler.java

lolodomo · 2022-10-31T10:59:12Z

...teminfo/src/main/java/org/openhab/binding/systeminfo/internal/handler/SysteminfoHandler.java

-            groupChannelsByPriority();
-            scheduleUpdates();
-            updateStatus(ThingStatus.ONLINE);
+            if (!addDynamicChannels()) { // If there are new channel groups, the thing will get recreated with a new


Looks like addDynamicChannels can return true without calling changeThingType. I have the feeling this case is not covered here ?

Indeed, I missed that. I don't think it can happen, as the computer thing type is the default one from the xml definition, but I added a thing status update to cope with it if it would.

lolodomo · 2022-10-31T11:36:53Z

End of review.

openhab-bot · 2022-10-31T13:23:20Z

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/oh-3-2-systeminfo-binding-process-pi4/140406/2

lolodomo · 2022-11-03T08:17:18Z

@mherwege : any chance you update the default translations, fix the typo and answer to the remaining question before Sunday?

Signed-off-by: Mark Herwege <mark.herwege@telenet.be>

mherwege · 2022-11-03T21:56:48Z

@lolodomo I did the adjustments. Thank you for the review.

lolodomo · 2022-11-03T22:35:53Z

...teminfo/src/main/java/org/openhab/binding/systeminfo/internal/handler/SysteminfoHandler.java

+            } else {
+                updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.HANDLER_INITIALIZING_ERROR,
+                        "@text/offline.cannot-initialize");
+                return false;


If you return false, this will trigger the additional code in initialize that finally sets the status to ONLINE.
So either you have to return true there or you return false but do not set the status to OFFLINE there.
Something looks not coherent.

@lolodomo Indeed, didn't think it through well enough. I think the new commit should fix that. Not setting the status here would force me to set it in the initialize method, and I don't know in initialize if the handler is being recreated. It shouldn't set it if it is being recreated.

Signed-off-by: Mark Herwege <mark.herwege@telenet.be>

lolodomo

LGTM, thank you

* Dynamic channels * Status messages i8n * Format fix * Cache process load values * Restore channel configs * Fix test * Stabilize tests * Fix CpuLoad1-5-15 update * Fix test bndrun * String equals cleanup * Fix potential null pointer in test Signed-off-by: Mark Herwege <mark.herwege@telenet.be>

openhab-bot · 2022-11-12T16:30:57Z

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/openhab-3-4-milestone-discussion/138093/98

lolodomo · 2022-11-12T16:40:46Z

@mherwege : could you please have a look to the above community topic, it looks like your PR broke the binding... for at least one user.

mherwege · 2022-11-12T19:18:45Z

@mherwege : could you please have a look to the above community topic, it looks like your PR broke the binding... for at least one user.

@lolodomo First analysis hints it may be linked to a file based thing definition. I don’t immediately see a way to fix that, and I am short in time at the moment to further analyse. I will do so later. Would you know if there is any way to detect if the definition was file based? If so, I could disable dynamic channels if one uses file thing definitions.

* Dynamic channels * Status messages i8n * Format fix * Cache process load values * Restore channel configs * Fix test * Stabilize tests * Fix CpuLoad1-5-15 update * Fix test bndrun * String equals cleanup * Fix potential null pointer in test Signed-off-by: Mark Herwege <mark.herwege@telenet.be>

mherwege requested a review from svilenvul as a code owner October 17, 2022 20:11

mherwege added 6 commits October 18, 2022 09:19

Dynamic channels

a823c57

Signed-off-by: Mark Herwege <mark.herwege@telenet.be>

Status messages i8n

b1cfe60

Signed-off-by: Mark Herwege <mark.herwege@telenet.be>

Format fix

168ab4d

Signed-off-by: Mark Herwege <mark.herwege@telenet.be>

Cache process load values

6755394

Signed-off-by: Mark Herwege <mark.herwege@telenet.be>

Restore channel configs

17e7261

Signed-off-by: Mark Herwege <mark.herwege@telenet.be>

Fix test

88d7069

Signed-off-by: Mark Herwege <mark.herwege@telenet.be>

Stabilize tests

8ed6435

Signed-off-by: Mark Herwege <mark.herwege@telenet.be>

mherwege force-pushed the systeminfo_channels branch from 614d5d0 to 8ed6435 Compare October 19, 2022 07:26

Fix CpuLoad1-5-15 update

3ccf4c9

Signed-off-by: Mark Herwege <mark.herwege@telenet.be>

mherwege added 3 commits October 20, 2022 11:45

Fix test bndrun

821b5c7

Signed-off-by: Mark Herwege <mark.herwege@telenet.be>

String equals cleanup

f071125

Signed-off-by: Mark Herwege <mark.herwege@telenet.be>

Fix potential null pointer in test

eeeecb3

Signed-off-by: Mark Herwege <mark.herwege@telenet.be>

lolodomo requested changes Oct 30, 2022

View reviewed changes

lolodomo added enhancement An enhancement or new feature for an existing add-on (potentially) not backward compatible and removed (potentially) not backward compatible labels Oct 31, 2022

lolodomo requested changes Oct 31, 2022

View reviewed changes

Review adjustments

3d316da

Signed-off-by: Mark Herwege <mark.herwege@telenet.be>

lolodomo requested changes Nov 3, 2022

View reviewed changes

Fix initialization status update

99dc2a3

Signed-off-by: Mark Herwege <mark.herwege@telenet.be>

lolodomo approved these changes Nov 4, 2022

View reviewed changes

lolodomo added rebuild Triggers Jenkins PR build and removed rebuild Triggers Jenkins PR build labels Nov 4, 2022

lolodomo merged commit cf2a1af into openhab:main Nov 4, 2022

lolodomo added this to the 3.4 milestone Nov 4, 2022

lolodomo mentioned this pull request Nov 13, 2022

[systeminfo] broken when thing defined in text file #13708

Closed

mherwege mentioned this pull request Dec 10, 2022

[systeminfo] SysteminfoOSGiTest unstable #13524

Open

mherwege deleted the systeminfo_channels branch December 11, 2022 11:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[systeminfo] dynamic channels #13562

[systeminfo] dynamic channels #13562

mherwege commented Oct 17, 2022

lolodomo commented Oct 19, 2022

mherwege commented Oct 19, 2022 •

edited

mherwege commented Oct 20, 2022

lolodomo commented Oct 20, 2022

mherwege commented Oct 20, 2022 •

edited

lolodomo left a comment •

edited

lolodomo commented Oct 30, 2022

jaywiseman1971 commented Oct 30, 2022

mherwege commented Oct 30, 2022

lolodomo commented Oct 31, 2022

lolodomo left a comment

lolodomo Oct 31, 2022

mherwege Nov 3, 2022

lolodomo commented Oct 31, 2022

openhab-bot commented Oct 31, 2022

lolodomo commented Nov 3, 2022

mherwege commented Nov 3, 2022

lolodomo Nov 3, 2022

mherwege Nov 4, 2022 •

edited

lolodomo left a comment

openhab-bot commented Nov 12, 2022

lolodomo commented Nov 12, 2022

mherwege commented Nov 12, 2022

[systeminfo] dynamic channels #13562

[systeminfo] dynamic channels #13562

Conversation

mherwege commented Oct 17, 2022

lolodomo commented Oct 19, 2022

mherwege commented Oct 19, 2022 • edited

mherwege commented Oct 20, 2022

lolodomo commented Oct 20, 2022

mherwege commented Oct 20, 2022 • edited

lolodomo left a comment • edited

Choose a reason for hiding this comment

lolodomo commented Oct 30, 2022

jaywiseman1971 commented Oct 30, 2022

mherwege commented Oct 30, 2022

lolodomo commented Oct 31, 2022

lolodomo left a comment

Choose a reason for hiding this comment

lolodomo Oct 31, 2022

Choose a reason for hiding this comment

mherwege Nov 3, 2022

Choose a reason for hiding this comment

lolodomo commented Oct 31, 2022

openhab-bot commented Oct 31, 2022

lolodomo commented Nov 3, 2022

mherwege commented Nov 3, 2022

lolodomo Nov 3, 2022

Choose a reason for hiding this comment

mherwege Nov 4, 2022 • edited

Choose a reason for hiding this comment

lolodomo left a comment

Choose a reason for hiding this comment

openhab-bot commented Nov 12, 2022

lolodomo commented Nov 12, 2022

mherwege commented Nov 12, 2022

mherwege commented Oct 19, 2022 •

edited

mherwege commented Oct 20, 2022 •

edited

lolodomo left a comment •

edited

mherwege Nov 4, 2022 •

edited